Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stardew Valley: Fix a bug in equals between Or and And rules #4326

Conversation

Jouramie
Copy link
Contributor

@Jouramie Jouramie commented Dec 3, 2024

What is this fixing or adding?

So, I don't know how it went unnoticed for so long... Basically for Or and And rule, if two rules had the same combinable sub-rules, they would automatically be considered equals completely ignoring the other sub-rules. For example, a rule that require access to 5 regions would be considered equal to a different rule requiring 5 completely different regions. They would evaluate different, but be considered equal.

I found this while writing tests in another branch. This bug has been merged for ~9 months and no one noticed, so I guess the __eq__ is probably not used too often. At least the __hash__ is correctly implemented, so that probably saved a lot of problems.

How was this tested?

Not really tested, but it's kind of obvious that the __eq__ method should compare with other and not self.

If this makes graphical changes, please attach screenshots.

N/A

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Dec 3, 2024
Copy link
Member

@NewSoupVi NewSoupVi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WOW.

@NewSoupVi NewSoupVi merged commit 6896d63 into ArchipelagoMW:main Dec 3, 2024
16 checks passed
@Jouramie Jouramie deleted the StardewValley/fix-equals-in-aggregating-rules branch December 3, 2024 05:29
@agilbert1412
Copy link
Collaborator

image

Jouramie added a commit to agilbert1412/Archipelago that referenced this pull request Dec 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants